Skip to content

Conversation

alan412
Copy link
Collaborator

@alan412 alan412 commented Feb 22, 2025

Fixes #25

@alan412 alan412 requested a review from lizlooney February 22, 2025 18:05
export class ExtendedPythonGenerator extends PythonGenerator {
private currentModule: commonStorage.Module | null = null;
private mapWorkspaceIdToExportedBlocks: { [key: string]: Block[] } = Object.create(null);
protected methods_: {[key: string]: string} = Object.create(null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why protected instead of private?
Why the trailing _?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost always go for protected instead of private so it can be subclassed later.
The trailing _ was because I was copying the style from definitions_

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm the opposite. I go for the most restrictive visibility first and then make it more visible only if necessary.
If it needs to be subclassed later, it can be changed from private to protected then.
https://google.github.io/styleguide/tsguide.html#visibility

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree fully about not making things public by default (as the style guide says), protected seems to be better than private. Imagine the mess we would be in if Blockly had made everything private instead of protected. (I'll admit that this tendency of mine is because I am almost always creating libraries or systems instead of applications.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense for libraries where you (as author of the library) want to allow a class to be subclassed. In this case, it is not a library and within this application, we don't currently want to subclass ExtendedPythonGenerator.

I'm going to approve (and merge) this PR so we can keep making progress.

@lizlooney lizlooney merged commit 04ec144 into wpilibsuite:main Feb 26, 2025
1 check passed
@alan412 alan412 deleted the remove_duplicate_comment branch March 21, 2025 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comment on method shows up twice
2 participants